[iOS] Removed locks && ensure everything runs on JS Thread #25164
Conversation
facebook-github-bot
added
the
CLA Signed
|
There was a problem hiding this comment.
@sammy-SC has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
React/Modules/RCTTiming.m
Outdated
| } | ||
|
|
||
| // Called from main thread |
There was a problem hiding this comment.
Could you please use RCTAssertMainQueue() (and co) instead of comments?
There was a problem hiding this comment.
@shergin 🤔 Emm, actually, these two method can be called on any threads, because we always dispatch work to JS Thread via dispatchBlock:queue. Should we need to add it?
There was a problem hiding this comment.
I removed the comments. We allow it to be called on any thread. :)
React/Modules/RCTTiming.m
Outdated
| @@ -171,28 +171,35 @@ - (dispatch_queue_t)methodQueue | |||
| return RCTJSThread; | |||
| } | |||
|
|
|||
| // Called from JS Thread. | |||
There was a problem hiding this comment.
I am concerned that it's always true.
There was a problem hiding this comment.
🤔 For the old module system, seems we use this rule to do things, for example : https://github.com/facebook/react-native/blob/master/React/Modules/RCTUIManager.m#L97-L101,
.... , but yeah TM may change it which callinvalidate always on JS Thread. I'll add an dispatch here. :)
zhongwuzw
requested review from
sammy-SC and
facebook-github-bot
and removed request for
facebook-github-bot
|
@shergin Yes, it has, more info please see #23674, #21211 (comment), the thread issue may happened when app go into background, open a dialog or something, the reason is we add the notification to observe app state, and its handler would be called on main thread, but we need to put all things to JS thread. Scheduling the block to JS thread only happened in cases like app goes into background, it's the rare case, most of the time, for the app running in foreground, we always timing on JS thread, not need any locks. |
|
This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
github-actions
bot
added
the
Stale
|
This PR was closed because it has been stalled for 7 days with no activity. |
Summary
Before, we add the lock to prevent race condition, it would happen because there has two threads can access concurrently, main thread and JS Thread, actually, most of the time, only JS Thread would call it, and main thread would access it only when app state changed, like in/out background.
Now, we only need to dispatch operations to JS Thread when app state changed, then we can remove all locks. Timer module is time sensitive, so I think remove locks is better. :)
Changelog
[iOS] [Enhancement] - Removed locks && ensure everything runs on JS Thread
Test Plan
CI passed.